Skip to content

Implement Image and Table support + Laravel Pint addition #5

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Implement Image and Table support + Laravel Pint addition #5

wants to merge 7 commits into from

Conversation

@igorsgm
Copy link

@igorsgm igorsgm commented Jun 26, 2025 •
edited
Loading

Hello @roelmagdaleno. Thank you for this useful package. I hope my PR will be a valuable contribution 🙏

Summary

  • Implemented Notion Image handling with dedicated processor and fallback external URL detection
  • Implemented Notion Table handling
  • Improved CommonMark integration with updated dependencies and better block rendering
  • Added comprehensive code formatting standards with Laravel Pint

Changes

  • Image Processing: Added MarkdownImageProcessor for better image URL handling and file validation
  • Updated Dependencies: Upgraded CommonMark to v2.7.0, Pest to v3.8.2, and added Laravel Pint v1.22
  • Code Quality: Implemented strict formatting rules with comprehensive Pint configuration
  • Testing: Added extensive image processing tests and improved test coverage
  • UTF-8 Handling: Enhanced invalid character validation to prevent encoding issues

The changes maintain backward compatibility while significantly improving the package's robustness and code quality standards.

igorsgm added 4 commits June 25, 2025 15:35
@igorsgm
Conversion improvements (#1)
2ceb35f 
* Bump dependencies + installing Laravel Pint

* .gitignore added

* Opinionated pint.json from nunomaduro/essentials added

* Pest test fix

* Add image processing and update annotations handling
@igorsgm
Conversion improvements (#2)
30c0c36 
* Bump dependencies + installing Laravel Pint

* .gitignore added

* Opinionated pint.json from nunomaduro/essentials added

* Pest test fix

* Add image processing and update annotations handling
@igorsgm
Conversion improvements (#3)
805d233 
* Bump dependencies + installing Laravel Pint

* .gitignore added

* Opinionated pint.json from nunomaduro/essentials added

* Pest test fix

* Add image processing and update annotations handling

* bump version

* Pest tests implemented
@igorsgm
Image validation (#4)
18100fa 
* wip

* wip
@roelmagdaleno
Copy link
Owner

roelmagdaleno commented Jun 27, 2025

Hey @igorsgm, I'll review it this weekend. Thank you.

@roelmagdaleno roelmagdaleno self-requested a review June 28, 2025 21:49
@roelmagdaleno roelmagdaleno added the enhancement New feature or request label Jun 28, 2025
@igorsgm igorsgm changed the title Implement Image support + Laravel Pint addition Implement Image and Table support + Laravel Pint addition Jun 29, 2025
roelmagdaleno
roelmagdaleno requested changes Jun 30, 2025
View reviewed changes
Copy link
Owner

@roelmagdaleno roelmagdaleno left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@igorsgm Thanks for this PR.

Adding the image and table components are good features.

I've reviewed all files and found some code that could be improved, let me know your thoughts about it. You'll find my comments on the files.

Also, if possible, let's use defensive programming with guard clauses. That means to remove the else statements and nested if conditions. We're trying to avoid the callback/condition hell and improve code readability.

Here's a good article that explains it all.

Thank you. Let me know any feedback.

src/Validation/ImageValidator.php
Copy link
Owner

@roelmagdaleno roelmagdaleno Jun 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@igorsgm The ImageValidator class looks good. I'd move the class to a Validators folder instead of Validation. The ImageValidator class has the word Validator, so I expect that to live inside a folder with the same name.

This is more for a good developer experience. Let's keep a consistency between class names and folders.

@igorsgm
Image validation refactoring and code improvements
4aa2e25 
- Bump minimum PHP version requirement from 8.0 to 8.1 for enhanced language features (enum)
- Refactor image validation by creating ImageType enum to centralize image block creation logic
- Convert ImageValidator from static class to instance-based with proper encapsulation
- Improve code readability with early returns and guard clauses in MarkdownImageProcessor and NotionBlocksRenderer
- Move ImageValidator from Validation to Validators namespace for better organization
- Optimize control flow structures using continue statements to reduce nesting depth
- Make RichText defaultObject method public static for better reusability across components
- Enhance table processing logic with cleaner conditional handling in Table class
- Update constructor injection patterns to use readonly properties for better performance
- Rename ImageValidationTest to ImageValidatorTest to match new class structure
@igorsgm
Copy link
Author

igorsgm commented Jul 1, 2025 •
edited
Loading

Hi @roelmagdaleno,

Thank you for the excellent feedback on this PR! I've successfully addressed all your suggestions:

✅ Changes Made:

1. Renamed detectHasHeader to hasHeader in Table.php

  • Improved method naming for better readability

2. Moved ImageValidator to Validators/ folder

  • Better consistency between class names and folder structure
  • Updated all imports accordingly

3. Converted ImageValidator from static to instance methods

  • Added private $validator property to Image class
  • Created isValid() method as suggested
  • Eliminated duplicate $isExternalUrl logic

4. Created ImageType enum with specialized methods

  • Added INVALID, EXTERNAL, FILE cases
  • Moved caption logic to enum using RichText::defaultObject()

5. Applied defensive programming with guard clauses

  • Eliminated nested if-else statements throughout codebase
  • Applied early returns to reduce nesting
  • Updated: NotionBlocksRenderer.php, MarkdownImageProcessor.php, ImageValidator.php, Table.php

📝 Regarding the Image Processor Placement:

The image processor in NotionBlocksRenderer serves a specific architectural purpose. It needs to analyze CommonMark
nodes during the rendering phase to:

  • Extract images from complex nested node structures
  • Handle image-like links (like ![alt](url) syntax)
  • Skip paragraphs that contain only images
  • Access internal CommonMark node data during conversion

Moving it outside would require exposing internal CommonMark node structures and complicate the rendering pipeline.
The current design maintains clean separation of concerns while allowing necessary access to node data during the
conversion process.

✅ Results:

  • All 82 tests pass - no breaking changes
  • Better design patterns with improved separation of concerns
  • Reduced complexity through guard clauses
  • Enhanced maintainability with consistent RichText::defaultObject() usage

Thank you for the thoughtful review and excellent suggestions!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@roelmagdaleno roelmagdaleno roelmagdaleno requested changes

Assignees

@igorsgm igorsgm

Labels

enhancement New feature or request

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@igorsgm @roelmagdaleno